Hi @aamotharald, thanks for your contribution!

Unfortunately, this approach doesn't work, because the AbstractMojoTestCase extends junit.framework.TestCase which is a JUnit 4 class. We had tried to get rid of JUnit 4 before, but didn't manage to do it within reasonable efforts. However, with the current solution tests are fully executed using the JUnit 5 runner. The JUnit 4 dependency is only declared to prevent version conflicts, not for executing tests.

Aside from that, I don't understand why the resilience tests got changed, they are already using JUnit 5 and thus should have remain unchanged?

Finally, maybe you can split out the improvements to productive code, then it'll be much easier to understand them + the related test changes, compared to JUnit related test changes.

Hi @MatKuhr ,

still formatting ..

I see so the maven-plugin-testing-harness pulls in JUnit4.

At least with this restructuring once the testing-harness starts to support JUnit5 the switch will be done directly as I got rid of the JUnit4 rules which are not supported anymore in JUnit5 .

Hi @MatKuhr ,

Aside from that, I don't understand why the resilience tests got changed, they are already using JUnit 5 and thus should have remain unchanged?
I have removed the resilience tests changes. The automatic refactoring did something strage (added the @nested Annotation) which the required some changes on top.

Here marked the changes to production code with regards to Mojo parameters initialization:
https://github.com/SAP/cloud-sdk-java/pull/646/files#diff-4003e606fc15367c1dc753ab726025e50332e1b007c1c17b40d8378bf66a736b
https://github.com/SAP/cloud-sdk-java/pull/646/files#diff-c045461f9e02233bbf4fc2bb0762a3bb30247ca140c4f3c09289264d90ff9f87
https://github.com/SAP/cloud-sdk-java/pull/646/files#diff-a7728b1c4dcc9b610c4ca0d80a0ac53e9edbca165fc46a494051624b95bcc0d2
It seems to me that due to the @parameter Annotation dealing with null values is only needed for tests, as in production the default value will get applied and null can't be set for the MoJo..
E.g.: @Parameter( property = "openapi.generate.skip", defaultValue = "false" ) is not applied for test execution ..

The only advantage I see in this change here is that junit4 does not get pulled explicitly, but transitively.
That means that future changes in the maven-plugin-testing-harness will not case dependency incompatibility issues.
Looking at the activity around the repo (last change 2014) I doubt that the testing support in maven-plugin-testing-harness will soon also offer an abstract Base class based on JUnit 5..

Seems like maven-plugin-testing-harness 4.x.x is going for JUnit5 support with a JUnit5 Extension instead of inheriting from a base class.

@ExtendWith(MojoExtension.class)

Hence once we have a non-beta 4.0.0 release the tests would have to get refactored accordingly anyhow :-(

Hi @MatKuhr ,

I did some more research and think it makes sense to close my PR.

I have requested in ths ASF JIRA to have the testing-harness 3.3.4 released which offers JUnit5 support.

Then the plugin testing could be done like this:

package com.sap.cloud.sdk.datamodel.odata.generator;

import com.sap.cloud.sdk.datamodel.odata.generator.annotation.DefaultAnnotationStrategy;
import com.sap.cloud.sdk.datamodel.odata.utility.NameSource;
import com.sap.cloud.sdk.datamodel.odata.utility.S4HanaNamingStrategy;
import org.apache.maven.plugin.logging.Log;
import org.apache.maven.plugin.testing.junit5.InjectMojo;
import org.apache.maven.plugin.testing.junit5.MojoTest;
import org.junit.jupiter.api.Test;

import javax.inject.Inject;

import static org.assertj.core.api.SoftAssertions.assertSoftly;
import static org.junit.jupiter.api.Assertions.assertEquals;

@MojoTest
public class DataModelGeneratorMojoJunit5Test {

    private static final String POM = "src/test/resources/DataModelGeneratorMojoTest/pom-junit5.xml";

    @Inject
    private Log log;

    @Test
    @InjectMojo(goal = "generate", pom = POM)
    void simpleMojo(DataModelGeneratorMojo mojo) {
        assertEquals(log, mojo.getLog());
        final DataModelGenerator generator = mojo.getDataModelGenerator();

        assertSoftly(softly -> {
            softly.assertThat(generator.getInputDirectory().getName()).isEqualTo("myInputDir");
            softly.assertThat(generator.getOutputDirectory().getName()).isEqualTo("myOutputDir");
            softly.assertThat(generator.isDeleteTargetDirectory()).isTrue();
            softly.assertThat(generator.isForceFileOverride()).isTrue();
            softly.assertThat(generator.getPackageName()).isEqualTo("my.package");
            softly.assertThat(generator.getDefaultBasePath()).isEqualTo("my/base/path/");
            softly
                    .assertThat(generator.getServiceNameMappings().getName())
                    .isEqualTo("myServiceNameMappings.properties");
            softly
                    .assertThat(generator.getNamingStrategy().getClass().getName())
                    .isEqualTo(S4HanaNamingStrategy.class.getName());
            softly.assertThat(generator.getNameSource()).isEqualTo(NameSource.NAME);
            softly
                    .assertThat(generator.getAnnotationStrategy().getClass().getName())
                    .isEqualTo(DefaultAnnotationStrategy.class.getName());
            softly.assertThat(generator.isGeneratePojosOnly()).isTrue();
            softly.assertThat(generator.getExcludeFilePattern()).isEqualTo("**/myExclusions/**");
            softly.assertThat(generator.isGenerateLinksToApiBusinessHub()).isTrue();
            softly.assertThat(generator.getIncludedEntitySets()).contains("entitySet1", "entitySet2");
            softly.assertThat(generator.getIncludedFunctionImports()).contains("fnImport1", "fnImport2", "fnImport3");
            softly.assertThat(generator.isFailOnWarning()).isTrue();
            softly.assertThat(generator.getCopyrightHeader()).isEmpty();
            softly.assertThat(generator.isServiceMethodsPerEntitySet()).isTrue();
        });    }

}

What do you think?

Thanks for requesting the update! Yeah, I think that would be the best solution for us 👍🏻 Then we can simply get rid of the current workaround and fully remove junit4 from our dependency tree.

Since you have access to that project, could you maybe ping here / re-open this PR once the update is available? Otherwise our dependabot should also catch the update eventually